Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve validation error display #1579

Merged
merged 6 commits into from
Jun 15, 2023
Merged

Improve validation error display #1579

merged 6 commits into from
Jun 15, 2023

Conversation

jjnesbitt
Copy link
Member

@jjnesbitt jjnesbitt commented Apr 24, 2023

This displays validation errors in a modal, instead of a small menu next to the sidebar. The version metadata validation errors remain largely unchanged. The asset validation errors have been improved to both display the path of the asset, as well as group the errors of an asset, if an asset contains multiple errors.

This is how it looks now:

image

@jjnesbitt
Copy link
Member Author

I'm aware of the test failures and will fix them soon.

@jwodder
Copy link
Member

jwodder commented May 15, 2023

CLI PR: dandi/dandi-cli#1288

@yarikoptic
Copy link
Member

@jwodder was proactive and prepared a PR for the change in API return values, but I wonder if such change is really needed? Ideally API (or ABI as the schema of returned structures) should not change without a significant reason since it would break prior versions of client(s) etc.
@AlmightyYakob would you be so kind to provide a summary of the change in returned data structure and why it cannot be avoided.

@jjnesbitt
Copy link
Member Author

@AlmightyYakob would you be so kind to provide a summary of the change in returned data structure and why it cannot be avoided.

Previously for asset validation errors, the API returned a flat list of errors (dicts), with no distinction as to which errors belonged to which assets. Since each asset can have more than one error, the format of dict[str, list[dict]] seemed the most efficient way to convey this.

@yarikoptic
Copy link
Member

@AlmightyYakob would you be so kind to provide a summary of the change in returned data structure and why it cannot be avoided.

Previously for asset validation errors, the API returned a flat list of errors (dicts), with no distinction as to which errors belonged to which assets. Since each asset can have more than one error, the format of dict[str, list[dict]] seemed the most efficient way to convey this.

ATM it does feel like that indeed. But in the long run, it might be "too restrictive": an error might relate to multiple assets, or to some entity over assets ("sub-X/ folder is not listed among participants.tsv" in BIDS), etc. Why not to just add path field and keep otherwise returning a list?
If it is desired for a viewer to group by path, it would remain a trivial operation, and could have any flexibility desired. FWIW, it would also relate nicely to our ValidationResult records which we might want to group by path or by error id or by some other criteria.

@waxlamp
Copy link
Member

waxlamp commented May 24, 2023

@AlmightyYakob would you be so kind to provide a summary of the change in returned data structure and why it cannot be avoided.

Previously for asset validation errors, the API returned a flat list of errors (dicts), with no distinction as to which errors belonged to which assets. Since each asset can have more than one error, the format of dict[str, list[dict]] seemed the most efficient way to convey this.

ATM it does feel like that indeed. But in the long run, it might be "too restrictive": an error might relate to multiple assets, or to some entity over assets ("sub-X/ folder is not listed among participants.tsv" in BIDS), etc. Why not to just add path field and keep otherwise returning a list? If it is desired for a viewer to group by path, it would remain a trivial operation, and could have any flexibility desired. FWIW, it would also relate nicely to our ValidationResult records which we might want to group by path or by error id or by some other criteria.

In the interest of reducing the blast radius from the changes we're making to support better validation error UX, we'll maintain a flat list structure, adding a path property. This will likely allow for closing dandi/dandi-cli#1288 while allowing the UI to add incremental value.

Your other arguments about restrictiveness point to a different issue, that it is becoming more and more difficult to mediate both the CLI's and web app's needs through a single public API. I have a bold idea here, but I need to think about it a bit more before I share my thoughts (perhaps in a design doc). Stay tuned.

@yarikoptic
Copy link
Member

Your other arguments about restrictiveness point to a different issue, that it is becoming more and more difficult to mediate both the CLI's and web app's needs through a single public API. I have a bold idea here, but I need to think about it a bit more before I share my thoughts (perhaps in a design doc).

I would like to hear some of such issues first before we even contemplate a possible redesigns etc. IMHO the proposed here idea of changing the Model (if we think in terms of MVC design pattern) was not really needed/desired regardless either it is for Web or CLI.

@yarikoptic
Copy link
Member

ping: conflicts

@jjnesbitt
Copy link
Member Author

@mvandenburgh @marySalvi This PR is ready for review.

@jwodder
Copy link
Member

jwodder commented Jun 8, 2023

@AlmightyYakob Please summarize the changes to the API that this PR currently makes.

@jjnesbitt
Copy link
Member Author

@AlmightyYakob Please summarize the changes to the API that this PR currently makes.

The only API change is that now the objects in the asset_validation_errors list include an extra path field, which represents the path of the asset that the error belongs to.

def inject_path(asset: dict, err: dict):
return {**err, 'path': asset['path']}

# Aggregate errors, ensuring the path of the asset is included
errors = []
for asset in invalid_assets:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will be really slow and/or memory intensive if a dandiset has hundreds of thousands of invalid assets. In general, I don't like how we go about storing asset validation errors in the database, but this PR is a strict improvement over the current situation imo. Perhaps related to #1214.

Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jjnesbitt jjnesbitt added patch Increment the patch version when merged release Create a release when this pr is merged labels Jun 15, 2023
@jjnesbitt jjnesbitt merged commit fc7850a into master Jun 15, 2023
@jjnesbitt jjnesbitt deleted the validation-error-display branch June 15, 2023 16:19
@dandibot
Copy link
Member

🚀 PR was released in v0.3.42 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants